-
Notifications
You must be signed in to change notification settings - Fork 893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[iOS] - Add Page Translation #25391
base: master
Are you sure you want to change the base?
[iOS] - Add Page Translation #25391
Conversation
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Show resolved
Hide resolved
...e/Frontend/UserContent/UserScripts/Scripts_Dynamic/Scripts/Sandboxed/BraveTranslateScript.js
Outdated
Show resolved
Hide resolved
The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "password, secure, insecure" and so security team members have been added as reviewers to take a look. |
@Brandon-T where the |
It comes from combining: https://source.chromium.org/chromium/chromium/src/+/main:components/translate/core/browser/resources/translate.js;l=5;drc=84091f477eff8e67261d6cdd9471519e8fee6185?q=translate&sq=&ss=chromium%2Fchromium%2Fsrc which downloads: https://translate.brave.com/static/v1/js/element/main.js We can't currently pull it from Brave-Core yet. |
886c0bc
to
e740285
Compare
ios/brave-ios/Sources/Brave/Frontend/Browser/Toolbars/UrlBar/File.swift
Outdated
Show resolved
Hide resolved
script | ||
.replacingOccurrences(of: "$<brave_translate_script>", with: namespace) | ||
.replacingOccurrences( | ||
of: "$<brave_translate_api_key>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we send requests to translate.brave.com from native and not from JS, can we append the API key in native here https://github.com/brave/brave-core/pull/25391/files#diff-b1da8ce8af5f2b9cdc18974acf1035386b4965f6a32689c077326939cb913453R520 ?
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Outdated
Show resolved
Hide resolved
func translate( | ||
_ request: BraveTranslateScriptHandler.RequestMessage | ||
) async throws -> (data: Data, response: URLResponse) { | ||
var urlRequest = URLRequest(url: request.url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we start pinning such requests? I know we don't do it for APIs but we need to start pinning imo. Could be a separate PR/issue.
@@ -0,0 +1,8438 @@ | |||
window.__firefox__ = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's impossible to review 8kb of minified/obfuscated JS. If it comes from Chromium - why is it obfuscated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and found each script + dependency manually in Chromium. Then I got the de-obfuscated version of those scripts and replaces the necessary code with it + added links to where the script was found.
This should drastically help as there's no more minified/obfuscated JS: 885cb0e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current approach looks extremely hard to support and review.
It's not clear why we have to bundle all the stuff into a one script. Why we can't use the desktop approach?
Also, the script from translate.brave.com is designed to be updatable from the backend side.
Option 1 (preferable): enable translate code from Chromium for iOS (or at least partially): https://source.chromium.org/chromium/chromium/src/+/main:components/translate/ios/browser/
It probably needs some chromium primitives like WebState, not sure about the current status wherever we support them or not.
Option 2: bundle all the supplementary scripts from b-c using webpack and put them to resources. Load the main script from https://translate.brave.com/, combine with the new script from resources and inject
@kylehickinson @StephenHeaps maybe you have ideas how to simplify things here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We don't support WebState yet, so for this PR it isn't possible to use the Desktop approach.
- If we load it from translate.brave.com, it doesn't help that it's still minified and unreadable and still impossible to review.
- Loading the scripts from translate.brave.com doesn't let us modify it to add Apple Translate support, which this PR does.
- How do we web-pack all the Chromium translate scripts and dependencies including all the ones specific to iOS that are in typescript, with Webpack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the aswers, @Brandon-T.
We don't support WebState yet, so for this PR it isn't possible to use the Desktop approach.
Ok, we can't reuse it as-is, but I mean to reuse the approach to build the script on the fly.
If we load it from translate.brave.com, it doesn't help that it's still minified and unreadable and still impossible to review.
It makes sense for the translate script itself, but not for all the script from /src.
The point is to have the same script for desktop & iOS, but different platform adapters.
If you want to improve the current translate script, it's stored here: https://github.com/brave/go-translate/tree/master/assets/static/v1
Loading the scripts from translate.brave.com doesn't let us modify it to add Apple Translate support
it's not clear to me.. If we download the script in advance and store locally, what is the difference with hardcoded script?
- How do we web-pack all the Chromium translate scripts and dependencies including all the ones specific to iOS that are in typescript, with Webpack?
Why we can't import them in a .ts file to transpile. That way we avoid duplicating the files.
Otherwise it's not clear how they will be synchronized in future.
Transpiling via transpile_web_ui
+ loading via a resource bundle (example) works for iOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I download it on the fly in the latest commit. So now it downloads the translate script instead of hard-coding.
transpile_web_ui
only works with WebUI. It doesn't work with just plain .ts files without any html.
https://github.com/brave/reviews/issues/1650#issuecomment-2223144564
It requires us to build resource files for iOS to export strings and other resources explicitly, when WebUI is not involved. We've had this discussion with Brian J. and a few others. Also it currently breaks unit tests. It's on the roadmap, but it's not ready yet, so for now it's not possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it currently breaks unit tests.
Not sure specifics of what unit tests would break / how in this PR, but this sounds similar to unit tests in #25694. I will be updating that PR soon to allow loading the needed resources from BraveCore (we need to load procedural_filters.ts
to allow our ScriptExecutionTests
to use it & run again, some more info here). Not sure if that approach helps with unit test concerns here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted all the copied JS, and pulled it from Brave-Core: https://github.com/brave/brave-core/blob/0a452b206220fb6f5e629dc76c58a30ed4075a13/ios/browser/api/translate/translate_script.mm
via Features. We can't use WebState, and we can't use what CosmeticFilters are using as these scripts are already compiled.
...e/Frontend/UserContent/UserScripts/Scripts_Dynamic/Scripts/Sandboxed/BraveTranslateScript.js
Show resolved
Hide resolved
ab61b03
to
8278a9e
Compare
16f359e
to
7b39433
Compare
885cb0e
to
e88edba
Compare
...e/Frontend/UserContent/UserScripts/Scripts_Dynamic/Scripts/Sandboxed/BraveTranslateScript.js
Outdated
Show resolved
Hide resolved
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Show resolved
Hide resolved
...ntent/UserScripts/Scripts_Dynamic/ScriptHandlers/Sandboxed/BraveTranslateScriptHandler.swift
Show resolved
Hide resolved
...e/Frontend/UserContent/UserScripts/Scripts_Dynamic/Scripts/Sandboxed/BraveTranslateScript.js
Outdated
Show resolved
Hide resolved
injectionTime: .atDocumentEnd, | ||
forMainFrameOnly: true, | ||
in: scriptSandbox | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reported by reviewdog 🐶
[semgrep] WKUserScript usages should be vet by the security-team.
References:
- https://github.com/brave/brave-browser/wiki/Security-reviews (point 13)
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/brave-execute-script-ios.yaml
Cc @stoletheminerals @bridiver
0e7715c
to
1a05ca5
Compare
get { | ||
return _translateState | ||
} | ||
set(state) { | ||
_translateState = state | ||
switch _translateState { | ||
case .unavailable: | ||
self.isEnabled = false | ||
self.isSelected = false | ||
case .available: | ||
self.isEnabled = true | ||
self.isSelected = false | ||
case .pending: | ||
self.isEnabled = true | ||
self.isSelected = false | ||
case .active: | ||
self.isEnabled = true | ||
self.isSelected = true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the var split out like this? (_translateState
/translateState
) when you could just use didSet
here?
// TODO: Take from Brave-Core's list | ||
// TODO: Take from Apple's list | ||
private var languages: [Locale.Language] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO's still in place? Can you open issues to track and link them
} | ||
|
||
public class Coordinator { | ||
var presentedViewController: WeakRef<UIViewController>? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to use WeakRef
if its inside of a class and can just mark the type as weak
directly?
Image( | ||
uiImage: UIImage(named: "translate-onboarding-icon", in: .module, compatibleWith: nil)! | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to drop to UIImage
, you can just do Image("translate-onboarding-icon", bundle: .module)
Image( | ||
uiImage: UIImage(named: "translate-onboarding-icon", in: .module, compatibleWith: nil)! | ||
) | ||
.padding([.top, .bottom], 24.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: .padding(.vertical, 24)
@@ -269,6 +269,16 @@ extension Strings { | |||
) | |||
} | |||
|
|||
// Brave Translate | |||
extension Strings { | |||
public static let braveTranslateAvailableVoiceOverAnnouncement = NSLocalizedString( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to BraveStrings
} | ||
} | ||
|
||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: return [result copy];
tableName: "BraveShared", | ||
bundle: .module, | ||
value: "Translate", | ||
comment: "Share action title" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment needs updating
} | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these hanging semi-colons for something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google JS formatter was doing that. I removed all of them.
All Chromium JS also has the same thing lol.
} | ||
|
||
class BraveTranslateScriptHandler: NSObject, TabContentScript { | ||
private weak var tab: Tab? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to refactor this script handler to not hold onto a tab for the upcoming CWVWebView usage. The tab
will only be passed in the didReceiveScriptMessage
delegate method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, for this branch you can pass in a Tab and hold onto it, but we shouldn't use it anywhere outside of the didReceiveScriptMessage content controller delegate method (pass it to other methods) to ensure we can easily use the same script handler in the cwv-webview branch
return searchText.isEmpty | ||
? languages | ||
: languages.filter({ | ||
languageName(for: $0).lowercased().contains(searchText.lowercased()) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should start using https://developer.apple.com/documentation/foundation/nsstring/1409742-localizedstandardcompare to compare user input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry wrong link, should be https://developer.apple.com/documentation/foundation/nsstring/1416328-localizedstandardcontains
.navigationTitle(Strings.BraveTranslate.languageSelectionButtonTitle) | ||
.navigationBarTitleDisplayMode(.inline) | ||
.toolbar { | ||
ToolbarItem(placement: .navigationBarLeading) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
placement: .cancellationAction
Toggle(isOn: $translateEnabled.value) { | ||
Text(Strings.BraveTranslate.settingsTranslateEnabledOptionTitle) | ||
} | ||
.toggleStyle(SwitchToggleStyle(tint: .accentColor)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SwitchToggleStyle(tint:)
is deprecated, just replace with .tint(Color.accentColor)
1a05ca5
to
bede4f9
Compare
bede4f9
to
b34ec80
Compare
injectionTime: .atDocumentEnd, | ||
forMainFrameOnly: true, | ||
in: scriptSandbox | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reported by reviewdog 🐶
[semgrep] WKUserScript usages should be vet by the security-team.
References:
- https://github.com/brave/brave-browser/wiki/Security-reviews (point 13)
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/brave-execute-script-ios.yaml
Cc @stoletheminerals @bridiver
|
||
let braveTranslateScriptHandler = BraveTranslateScriptHandler(tab: tab, delegate: self) | ||
let braveTranslateTabHelper = BraveTranslateTabHelper(tab: tab, delegate: self) | ||
|
||
injectedScripts.append( | ||
BraveTranslateScriptLanguageDetectionHandler(tab: tab, delegate: braveTranslateScriptHandler) | ||
BraveTranslateScriptLanguageDetectionHandler(tabHelper: braveTranslateTabHelper) | ||
) | ||
injectedScripts.append(braveTranslateScriptHandler) | ||
injectedScripts.append(BraveTranslateScriptHandler(tabHelper: braveTranslateTabHelper)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the BraveTranslateTabHelper
to be held by the Tab
instead of the script handler, it shouldn't be owned by the script handler. Like I mentioned previously, the script handlers shouldn't be using the Tab
unless they're handling JavaScript events.
func addTabHelper(_ helper: TabHelper) { | ||
let helperName = Swift.type(of: helper).tabHelperName | ||
if tabHelpers[helperName] != nil { | ||
assertionFailure("Tab Helper: \(helperName) already attached to this tab.") | ||
} | ||
|
||
self.tabHelpers[helperName] = helper | ||
} | ||
|
||
func getTabHelper(named name: String) -> TabHelper? { | ||
self.tabHelpers[name] | ||
} | ||
|
||
func removeTabHelper(named name: String) { | ||
self.tabHelpers.removeValue(forKey: name) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets just dump all of this TabHelper
stuff and just use a standard property on Tab
since we're not even matching Chromium's definition of a tab helper at this point anyways, we can always move towards that direction later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of it removed. Added directly to Tab
.
6d8f27c
to
4417172
Compare
4417172
to
897ceb5
Compare
Implement Translation UI Add Translate Toast and Settings Add language selection view Improve language detection. Allow translation of ReaderMode pages! Download Brave Translate scripts on the fly, lazily.
…s when disabled so URL-Bar doesn't show the button when disabled
897ceb5
to
6bc61a9
Compare
[puLL-Merge] - brave/brave-core@25391 DescriptionThis PR introduces Brave Translate functionality to the iOS app. It adds the ability to translate web pages to different languages directly within the browser. The feature includes UI components for translation settings, language selection, and user notifications. Possible Issues
Security Hotspots
ChangesChanges
sequenceDiagram
participant User
participant BrowserViewController
participant Tab
participant BraveTranslateTabHelper
participant TranslateURLBarButton
participant BraveTranslateScriptHandler
participant TranslateScript
User->>BrowserViewController: Visits webpage
BrowserViewController->>Tab: Creates Tab
Tab->>BraveTranslateTabHelper: Initializes helper
BrowserViewController->>TranslateURLBarButton: Updates button state
User->>TranslateURLBarButton: Taps translate button
TranslateURLBarButton->>BraveTranslateTabHelper: Initiates translation
BraveTranslateTabHelper->>BraveTranslateScriptHandler: Executes translate script
BraveTranslateScriptHandler->>TranslateScript: Retrieves translation script
TranslateScript-->>BraveTranslateScriptHandler: Returns script
BraveTranslateScriptHandler-->>BraveTranslateTabHelper: Applies translation
BraveTranslateTabHelper-->>BrowserViewController: Updates UI
BrowserViewController-->>User: Displays translated page
|
injectionTime: .atDocumentEnd, | ||
forMainFrameOnly: true, | ||
in: scriptSandbox | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reported by reviewdog 🐶
[semgrep] WKUserScript usages should be vet by the security-team.
References:
- https://github.com/brave/brave-browser/wiki/Security-reviews (point 13)
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/brave-execute-script-ios.yaml
Cc @stoletheminerals @bridiver
Security Review
Summary
The tweaks are:
This allows us to intercept translate requests, and pass it to the Apple On Device translation. Everything else is a direct copy from Brave-Core (UNTIL we get Chromium WebViews finished. After that, there will be no need for such a giant script copy).
Resolves brave/brave-browser#40782
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Please note that Brave-Translate might not always work (iOS 17). This is entirely due to our backend!
Apple Translate should always work (iOS 18+).
Screen.Recording.2024-08-30.at.10.00.17.AM.mov